refactor: Centralize CORS business logic into Tangle middleware#65
Conversation
maxy-shpfy
left a comment
maxy-shpfy
left a comment
There was a problem hiding this comment.
LGTM, but havent had a chance to test
| # Parse the comma-separated list and strip whitespace from each origin | ||
| allowed_origins = [ | ||
| origin.strip() | ||
| for origin in cors_origins_str.split(",") |
There was a problem hiding this comment.
Do we want to add more sanity-checks (aka valid URLs) since it is a user-input?
There was a problem hiding this comment.
Great idea. I am sure this will help some people.
I have added some validation and useful messages when invalid formats are detected.
| ) | ||
|
|
||
|
|
||
| def setup_global_middleware(app: fastapi.FastAPI) -> None: |
There was a problem hiding this comment.
NIT: No idea what naming conventions usually are, but since the usage is middleware.setup_global_middleware , maybe we simplify to middlware.setup? Not sure what not "global" middleware we may have.
There was a problem hiding this comment.
I think setup is fine for now as well. Thank you!
Until we have a group of middleware to call, I am just calling setup_cors_middleware directly now (1 less function).
morgan-wowk
force-pushed
the
01-08-refactor_centralize_cors_business_logic_into_tangle_middleware
branch
from
3d90d50 to
9d18a3c
Compare
Thanks for reviewing. I have added tests to the PR now as well to verify the middleware is meeting expectations, then will test again on staging with actual requests. |
morgan-wowk
force-pushed
the
01-08-refactor_centralize_cors_business_logic_into_tangle_middleware
branch
from
9d18a3c to
07b49cb
Compare
|
The internal startup script already has CORSMiddleware (7 lines). |
Hey, This is a solution that supports multiple configurations and isn't exclusive to our internal script. It's just in general an improvement to our CORS for different backend setups. For example, running the backend at a different address / domain then the frontend, or a port that is not 3000. |
We have been discussing backend configurations separately and in the future we might remove the custom backend settings; That's what is still being discussed. We can always remove this later on if we strictly support only relative backends, but for now this is a quality of life change. Especially for me where I sometimes am running my backend at a non-relative path / different port. |
3 participants
Why is this change needed:
localhost:3000, backend onlocalhost:8000)